Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update permissionless paymaster tutorial & l1-l2 #47

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlbionaHoti
Copy link
Contributor

Description

  1. Fix copy and rewrote some parts for permissionless paymaster
  2. Fix copy for l1 to l2 guide

@AlbionaHoti AlbionaHoti requested a review from a team as a code owner August 23, 2024 17:39
@AlbionaHoti AlbionaHoti requested a review from itsacoyote August 23, 2024 17:40
@AlbionaHoti AlbionaHoti changed the title Fix/update permissionless paymaster tutorial Fix: update permissionless paymaster tutorial & l1-l2 Aug 23, 2024
Copy link

Visit the preview URL for this PR (updated for commit 8cc4e8c):

https://community-cookbook-staging--pr47-fix-update-permissio-3u5ffkca.web.app

(expires Fri, 30 Aug 2024 17:42:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1b876338aafcf55b4a02f1877984e116731756b1

@AlbionaHoti AlbionaHoti changed the title Fix: update permissionless paymaster tutorial & l1-l2 fix: update permissionless paymaster tutorial & l1-l2 Aug 23, 2024
Copy link
Contributor

@bxpana bxpana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the coding portion, but besides the one comment it looks good


3. The signer's key signs this paymaster data and returns the signature and signer address to the Dapp's frontend.

4. Paymaster address and required data with signature are added to the transaction blob in the frontend.

5. User gets transaction signature request pop-up on their wallet. **User only signs the transaction** and the transaction is sent on-chain.
5. The user receives the transaction signature request pop-up on their wallet. **The User only signs the transaction**, and it is sent on-chain.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change **The User to **The user for consistency

@@ -357,7 +364,7 @@ const preparePaymasterParam = async (account:any, estimateGas: BigNumber) =>{

// Estimate gas
const estimateGas = await contract.estimateGas.approve(spender,amount);
const [paymasterParams, maxFeePerGas, gasLimit] = await preparePaymasterParam(account, estimateGas);
const [account, paymasterParams, maxFeePerGas, gasLimit] = await preparePaymasterParam(account, estimateGas);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would lead to error.

// --------------
const tx = await contract.approve(spender, amount);

const [account, paymasterParams, maxFeePerGas, gasLimit] = await preparePaymasterParam(account, estimateGas);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would lead to error


2. Dapp calls the backend server or Zyfi API with relevant data to get the signer's signature.
- It is recommended that the signer's signing part is done on a secure backend server of the Dapp.
For the utmost security, it is recommended that the signer's signature be sent to Dapp's secure backend server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For the utmost security, it is recommended that the signer's signature be sent to Dapp's secure backend server.
For security best-practices, it is recommended to create the signer's signature on a secure backend server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants